Completed Onboarding Challenge #103
Conversation
lm75bd/lm75bd.c
Outdated
| errCode = i2cSendTo(devAddr, writeBuf, 1); | ||
| RETURN_IF_ERROR_CODE(errCode); |
There was a problem hiding this comment.
these lines can be combined to just be RETURN_IF_ERROR_CODE(i2cSendTo(devAddr, writeBuf, 1);
lm75bd/lm75bd.c
Outdated
| // read temperature from sensor | ||
| uint8_t readBuf[2] = {0}; | ||
| errCode = i2cReceiveFrom(devAddr, readBuf, 2); | ||
| RETURN_IF_ERROR_CODE(errCode); |
lm75bd/lm75bd.c
Outdated
|
|
||
| // set pointer register to 'temperature sensor' | ||
| uint8_t writeBuf[] = {0}; |
There was a problem hiding this comment.
since you know this should only be 1 uint8_t long, its best to explicitly initialize it to that size for readability
services/thermal_mgr/thermal_mgr.c
Outdated
| @@ -42,19 +42,43 @@ void initThermalSystemManager(lm75bd_config_t *config) { | |||
| } | |||
|
|
|||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | |||
| /* Send an event to the thermal manager queue */ | |||
| if (event != NULL) { | |||
| if (xQueueSend(thermalMgrQueueHandle, (void *) event, (TickType_t) 0) == pdPASS) | |||
There was a problem hiding this comment.
make sure that the queue has already been created
services/thermal_mgr/thermal_mgr.c
Outdated
|
|
||
|
|
||
| float temp; | ||
| error_code_t errCode = readTempLM75BD(LM75BD_OBC_I2C_ADDR, &temp); |
There was a problem hiding this comment.
receive and check the event first before reading temp, since right now even if you don't receive a measure temp command you'll be reading the temperature
services/thermal_mgr/thermal_mgr.c
Outdated
| } | ||
|
|
||
| static void thermalMgr(void *pvParameters) { | ||
| /* Implement this task */ | ||
| thermal_mgr_event_t eventBuf; |
There was a problem hiding this comment.
in general try to limit the scope of your variables to the smallest scope possible, so move this into the while block
| if (xQueueReceive(thermalMgrQueueHandle, &eventBuf, 10) == pdPASS) { | ||
| if (eventBuf.type == THERMAL_MGR_EVENT_MEASURE_TEMP_CMD) { | ||
| error_code_t errCode = readTempLM75BD(LM75BD_OBC_I2C_ADDR, &temp); | ||
| if (errCode == ERR_CODE_SUCCESS) |
There was a problem hiding this comment.
make sure to log the error if it was no successful
| addTemperatureTelemetry(temp); | ||
| } | ||
| else if (eventBuf.type == THERMAL_MGR_EVENT_INTERRUPT_CMD) { | ||
| error_code_t errCode = readTempLM75BD(LM75BD_OBC_I2C_ADDR, &temp); |
There was a problem hiding this comment.
check if the function call was successful or not
…emp function call was successful
services/thermal_mgr/thermal_mgr.c
Outdated
|
|
||
| return ERR_CODE_SUCCESS; | ||
| return ERR_CODE_INVALID_QUEUE_MSG; |
There was a problem hiding this comment.
If the thermal mgr queue handle is null, then it makes more sense to return ERR_CODE_INVALID_STATE instead since we are in a state where we have not intitialized the queue yet and it's not related to the queue msg itself
|
|
||
| return ERR_CODE_SUCCESS; | ||
| return ERR_CODE_INVALID_QUEUE_MSG; | ||
| } |
There was a problem hiding this comment.
Also note that when your doing multiple errror checks like this it's a lot cleaner to just use multiple if statements with early returns rather than a bunch of nested if statements. For example something like ```
if(errCodeCondition){
return ERR_CODE
}
if(otherPossibleError){
return OTHER_ERROR
}
// continue with the rest of the function knowing everything before was success without need an else statement
| @@ -0,0 +1 @@ | |||
| --- | |||
There was a problem hiding this comment.
your integration test run looks wrong
services/thermal_mgr/thermal_mgr.c
Outdated
| @@ -42,19 +42,50 @@ void initThermalSystemManager(lm75bd_config_t *config) { | |||
| } | |||
|
|
|||
| error_code_t thermalMgrSendEvent(thermal_mgr_event_t *event) { | |||
| /* Send an event to the thermal manager queue */ | |||
|
|
|||
| if (&thermalMgrQueueHandle != NULL) | |||
There was a problem hiding this comment.
should be == NULL instead of !=, same for below
Purpose
Onboarding
New Changes
Implemented temperature sensor driver function, thermal management task, and OS handler.
Testing
Passed all unit and integration tests